Repository navigation
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
386ec37 to
c72e920
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request implements session resumption for resumable uploads in the Java GAX library. It introduces the resumeCall method in ResumableUploadCallableImpl, updates ResumableUploadFutureImpl and ResumableUploadProgressTracker to handle resumption, and enhances RewindableStreamBuffer to skip forward to the server's committed offset. Comprehensive unit and integration tests are also added. Feedback suggests preserving the original EOFException as the cause when catching it from ByteStreams.skipFully in RewindableStreamBuffer to maintain the exception chain and avoid potential null messages.
c72e920 to
cf0c9d4
Compare
c6f9b2b to
b02ed8c
Compare
| recoveryAlgorithm, | ||
| ResumableUploadProgressTracker.createForResume(sessionUrl, options.getChunkSize())); | ||
| try { | ||
| future.resume(sessionUrl, payloadSupplier); |
There was a problem hiding this comment.
The future should already have access to both sessionUrl and payloadSupplier, I don't think we have to pass them in. In addition, it seems the main responsibility of future.resume is to create a coordinator, which we may want to inline here.
| queryStatusCallable, | ||
| options, | ||
| clientContext, | ||
| recoveryAlgorithm, |
There was a problem hiding this comment.
nit: We may want to refactor the constructor since it has 8 arguments now. For example, I see that recoveryAlgorithm is always created and passed to the coordinator, maybe we can create the recoveryAlgorithm in the coordinator directly.
| setUpGlobalTimeout(); | ||
| this.payload = checkNotNull(payloadSupplier.get(), "payload supplier returned null"); | ||
| ResumableUploadChunkCoordinator<ResponseT> coordinator = | ||
| createAndRegisterCoordinator(sessionUrl, options.getChunkSize()); |
There was a problem hiding this comment.
I think there might be a security concern if we use the sessionUrl as it is. If a malicious url being passed in, we would still try to attach auth token to that url and make a call regardless. Is there anyway we can validate the url? Maybe it's host needs to match endpoint from client settings? Also this seems a cross-language concern, do we have any guidance?
There was a problem hiding this comment.
Put out a question to the cross-language working group about the general topic. By my reading of the internal docs I suspect that the host of the upload URL isn't guaranteed to be the same as the endpoint where the start command is sent though so I'm not sure what validation is feasible.
Another thought - IIUC the auth headers are only needed for the start command, so perhaps we could unset those in the wire-level callables we use for upload and query.
b02ed8c to
926d744
Compare
b8d38ad to
5563dbb
Compare
5563dbb to
a83bf70
Compare
a83bf70 to
a8d272c
Compare
|
|
Implements
ResumableUploadCallable.resumeCall(...)to support resuming interrupted resumable upload sessions from a saved session URL and effective chunk size.